Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Handle the DataCount section #264

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Conversation

alexcrichton
Copy link
Contributor

This is a relatively recent addition to the bulk-memory-ops proposal

This is a relatively recent addition to the bulk-memory-ops proposal
@NikVolf
Copy link
Contributor

NikVolf commented Jan 27, 2019

But isn't this going to make parity-wasm incompatible with v1 wasm?

@alexcrichton
Copy link
Contributor Author

I believe the intention is that if the section is missing that just means there are no passive data segments which should be compatible with v1 wasm, but as with all new features if used it's incompatible.

I think that compatibility story is the same here, although I could be wrong! I believe though that this PR retains the same level of compatibility as before

@pchickey
Copy link
Contributor

I believe that the issue is that your patch introduces DataCount as section 10 and moves Code and Data from 10 to 11 and 11 to 12.

The proposal introduces DataCount as section 12, which does not cause this conflict, but does introduce a new ordering rule about putting DataCount (if present) before the code and data sections in the binary.

@alexcrichton
Copy link
Contributor Author

The intention was to change the order to have DataCount as section 12, but not the ids. I've run this through wasm-bindgen's test suite and it doesn't break tests, which exercises wasm implementations of Node/Firefox/Chrome, so I'm relatively confident at least that this is not a breaking change

@NikVolf NikVolf merged commit 87df081 into paritytech:master Feb 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants